Automatic Error Collection #603
Conversation
The server running is using a local dinky server, pls no abooze. TODO: Implement Sentry catcher to ALL EXCEPTION HOLY THAT IS A LOT
`USEREMOTELOGGING`, automatically enabled on Preview & Debug branches for obvious reasons. update logger types
Does not initialize or de-initialize the utility. Note: An app restart may be required when selecting the option
sends it to Sentry so we don't have to refactor all our exception calls
Move SentrySDK init to its own helper
…nts' into sentry-error-collection
This added current log for when exception is sent to Dsn
1. Redacted username in the log file when its not debug build 2. Properly dispose SentrySdk when toggled off 3. Pipe SentrySdk log to Logger 4. Use correct flusher for SentryHelper.cs 5. Fixed cleaner loop for ExceptionHandler_ForLoop by @neon-nyan Co-authored-by: Kemal Setya Adhi <[email protected]>
No need for adding Sentry's ExceptionHandler on catcher that has SendException already
Qodana for .NET17 new problems were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at [email protected]
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Note: - Some submodules cannot import SentryHelper, which is in `Hi3Helper.Core` - Some calls are asynchronous, mostly for the handled exceptions, but also where it felt best to use them, feel free to change them if needed - Most are categorized as `UnhandledOther`, but if we feel the need to change some, we can, it simply affects tagging on the frontend dashboard.
|
Awaiting resolution for getsentry/sentry-dotnet#3747 |
1. Only uploads log file when exception is unhandled 2. Add cache dir path using Collapse's LocalLow data path 3. Dispose _loopToken before creating a new one to prevent memory leak 4. Reordered LogWriteLine on initialization 5. Release exception redirect event when feature is disabled
Also add a way to disable log uploading. This is due to a bug(?) where log upload length reported as 0 and causes BadRequest error. See getsentry/sentry-dotnet#3747
|
Ready for review Notes:
|
|
Alternatively, we could try to apply for Sentry's OSS program https://sentry.io/for/open-source/
Please anyone (devs or users) to state your opinion in this thread |
I think IMO it's worth applying for Sentry's OSS program, simply because we're using their SDK. A solution could be that we have 2 endpoints, 1 primary and 1 fallback, but at that point we'd need to implement more checks to select 1 automatically, and that's 2 different platforms so I don't know how pertinent that is. Overall, applying is worth it, we lose nothing by trying. For point 2, I don't think it's a big deal, we also have Edit: Some notable benefits for using Sentry is that we won't need to rely on our own equipment which brings the added benefit that we'll have 100% uptime for the feature. |
1. Use Sentry's own backend Still at testing phase, if any change happens it will be changed again to our self-hosted instance. 2. Add a way to disable via system environment variable Setting env var "DISABLE_SENTRY" to true or 1 will disable exception handling 3. Omit IP address from being sent 4. Regionalized SentryHandler.cs 5. Updated PRIVACY.md
| throw ex.Flatten().InnerExceptions.First(); | ||
| var innerExceptionsFirst = ex.Flatten().InnerExceptions.First(); | ||
| SentryHelper.ExceptionHandler(innerExceptionsFirst, SentryHelper.ExceptionType.UnhandledOther); | ||
| throw innerExceptionsFirst; |
There was a problem hiding this comment.
I don't think throwing AggregateException into Sentry will be a good idea since it can actually be another type exception like OperationCancelledException or TaskCancelledException. In my honest opinion, I think we need to check the innerExceptionsFirst first to check if it's no other than OperationCancelledException or TaskCancelledException, then just throw it. Other than that, just ignore it
| throw ex.Flatten().InnerExceptions.First(); | ||
| var innerExceptionsFirst = ex.Flatten().InnerExceptions.First(); | ||
| SentryHelper.ExceptionHandler(innerExceptionsFirst, SentryHelper.ExceptionType.UnhandledOther); | ||
| throw innerExceptionsFirst; |
| throw ex.Flatten().InnerExceptions.First(); | ||
| var innerExceptionsFirst = ex.Flatten().InnerExceptions.First(); | ||
| SentryHelper.ExceptionHandler(innerExceptionsFirst, SentryHelper.ExceptionType.UnhandledOther); | ||
| throw innerExceptionsFirst; |
| catch | ||
| catch (Exception ex) | ||
|
|
||
| { | ||
| await SentryHelper.ExceptionHandlerAsync(ex, SentryHelper.ExceptionType.UnhandledOther); | ||
| IsRunning = false; | ||
| throw; |
There was a problem hiding this comment.
This catch might probably throw OperationCancelledException or TaskCancelledException too due to the usage of Game Repair mechanism for checking the game integrity before the delta-patch come into play.
| catch (Exception ex) | ||
| { | ||
| await SentryHelper.ExceptionHandlerAsync(ex, SentryHelper.ExceptionType.UnhandledOther); | ||
| LogWriteLine($"Error has occurred while performing delta-patch!\r\n{ex}", LogType.Error, true); | ||
| throw; |
There was a problem hiding this comment.
This catch might also throw OperationCancelledException or TaskCancelledException. Type check is needed to exclude these two cancellation exceptions from sentry
| catch (Exception ex) | ||
| { | ||
| await SentryHelper.ExceptionHandlerAsync(ex, SentryHelper.ExceptionType.UnhandledOther); | ||
| throw; |
There was a problem hiding this comment.
Need cancellation exception check
| await SentryHelper.ExceptionHandlerAsync(ex, SentryHelper.ExceptionType.UnhandledOther); | ||
| LogWriteLine($"Failed while getting CDN content from: {cdnProp.Name} (prefix: {cdnProp.URLPrefix}) (relPath: {relativeURL})\r\n{ex}", LogType.Error, true); | ||
| return null; |
| await SentryHelper.ExceptionHandlerAsync(ex, SentryHelper.ExceptionType.UnhandledOther); | ||
| LogWriteLine($"Failed while getting CDN content from: {cdnProp.Name} (prefix: {cdnProp.URLPrefix}) (relPath: {relativeURL})\r\n{ex}", LogType.Error, true); | ||
| return false; |
| await SentryHelper.ExceptionHandlerAsync(ex, SentryHelper.ExceptionType.UnhandledOther); | ||
| LogWriteLine($"Failed while getting CDN content from: {cdnProp.Name} (prefix: {cdnProp.URLPrefix}) (relPath: {relativeURL})\r\n{ex}", LogType.Error, true); | ||
| return false; |
| await SentryHelper.ExceptionHandlerAsync(ex, SentryHelper.ExceptionType.UnhandledOther); | ||
| LogWriteLine($"CDN content from: {cdnProp.Name} (prefix: {cdnProp.URLPrefix}) (relPath: {relativeURL}) has failed to initialize due to an exception:\r\n{ex}", LogType.Error, true); | ||
| return CDNUtilHTTPStatus.CreateInitializationError(); |
1. Use the InnerException if exception caught is an AggregateException 2. Throw TCE/OCE to the bin and not upload them 3. Deduplicate exception handling mechanism and use an inner method instead
Fixed in 653ed45 by returning all ExceptionHandler method if it sees an OCE/TCE being caught and send them straight out to the bin, also the handler will use the first exception instead if it sees AggregateException being caught |
Also set the environment as either Debug/non-debug based if there is any debugger attached
|
Approval received. Merging to main... We also need to notify users about the privacy policy changes and let them know the way to disable sentry's mechanism can be done either by
|
Main Goal
Add a way for Collapse to automatically report errors to a self-hosted instance of SentrySDK compatible server
PR Status :
TODO
Add hard crash detectionUnnecessaryAdd ExceptionHandler to EVERY METHOD CATCHERsDone